fix(tf): keep linear-model loss configs after frozen components#5701
Conversation
LinearModel.get_loss reassigned its `loss` argument to each submodel's
return value. Frozen and pairtab submodels return None from get_loss, so when
such a non-trainable submodel preceded a trainable energy submodel, the next
submodel was called with loss=None and EnerFitting.get_loss crashed on
None.pop("type").
Iterate with a separate variable and pass a deepcopy of the original loss
config to each submodel, returning the first non-None result. Add a test with
a None-returning submodel ordered before a trainable one.
Fix deepmodeling#5682
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLinearModel.get_loss() in the TensorFlow linear model now deep-copies the loss configuration for each submodel invocation instead of reusing a shared reference, returning the first non-None submodel loss. A new unit test verifies the original loss config remains unmutated after the call. ChangesPreserve loss config across linear model submodels
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant LinearModel
participant FrozenSubmodel
participant EnergySubmodel
Caller->>LinearModel: get_loss(loss, lr)
LinearModel->>FrozenSubmodel: get_loss(copy.deepcopy(loss), lr)
FrozenSubmodel-->>LinearModel: None
LinearModel->>EnergySubmodel: get_loss(copy.deepcopy(loss), lr)
EnergySubmodel-->>LinearModel: loss_result
LinearModel-->>Caller: loss_result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5701 +/- ##
==========================================
- Coverage 81.97% 81.78% -0.19%
==========================================
Files 959 959
Lines 105748 105749 +1
Branches 4102 4106 +4
==========================================
- Hits 86684 86486 -198
- Misses 17573 17769 +196
- Partials 1491 1494 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
njzjz-bot
left a comment
There was a problem hiding this comment.
Looks good to me. The change preserves the original loss configuration for each linear submodel, correctly skips non-trainable submodels returning None, and adds a focused regression test for the frozen/pairtab-before-trainable ordering. CI is green.
Reviewed by OpenClaw 2026.6.8 (844f405)
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Problem
LinearModel.get_loss()reassigned itslossargument to each submodel's returned value:Frozen and pairtab submodels return
Nonefromget_loss()because they own no trainable loss. When such a non-trainable submodel is ordered before a trainable energy submodel, the trainable submodel is then called withloss=None, andEnerFitting.get_loss()dereferences it as a dict vialoss.pop("type", ...), raisingAttributeError: 'NoneType' object has no attribute 'pop'during trainer construction — even though the original loss config could have been used.Fix
Iterate with a separate variable and pass a
copy.deepcopy(loss)of the original loss configuration to each submodel, returning the first non-Noneresult. This keeps the caller's loss config intact for the trainable submodel regardless of submodel ordering, and avoids mutating the shared config (EnerFitting.get_losspops keys from it).Test
source/tests/tf/test_linear_model.pycovered a linear model whose submodels never hit this ordering, so the crash path was unexercised. A new lightweightTestLinearModelGetLossconstructs aLinearEnergyModelwith aNone-returning submodel (mimicking frozen/pairtab) ordered before a trainable submodel (mimickingEnerFitting.get_loss'sloss.pop), and assertsget_lossreturns the trainable loss and leaves the original config unmutated. On the current code it fails with theNoneType ... poperror; after the fix it passes.Fix #5682
Summary by CodeRabbit
Bug Fixes
Tests